-
-
Notifications
You must be signed in to change notification settings - Fork 457
fix(security): Replace eval() with safe math expression parser to prevent code injection #926
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…vent code injection ## Summary Fix critical code injection vulnerability in calculator.ts by replacing unsafe eval() with a secure recursive descent math parser. ## Vulnerability Using eval() with user-provided expressions allows arbitrary JavaScript code execution. An attacker could execute malicious code through the calculator tool. ## Fix - Remove eval() completely - Implement tokenize() function for lexical analysis - Implement parseExpression() recursive descent parser - Support +, -, *, /, ** operators with proper precedence - Support parentheses for grouping - Only allow numeric values and mathematical operations ## Testing - Verified basic arithmetic: 2+2, 10-5, 3*4, 20/4 - Verified operator precedence: 2+3*4 = 14 - Verified exponentiation: 2**10 = 1024 - Verified parentheses: (2+3)*4 = 20 - Verified malicious input is rejected
|
📝 WalkthroughWalkthroughThe calculator tool was refactored to replace eval-based calculations with a safe tokenizer and recursive-descent parser supporting operator precedence, exponentiation, and unary operators. The public API remains unchanged while security is substantially improved through elimination of eval. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 1 file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/next-js-chatbot-starter-template/lib/tools/calculator.ts (1)
18-25: Consider validating number format to catch malformed decimals.The current implementation allows numbers like
1.2.3to be silently parsed as1.2due toparseFloatbehavior. While not a security issue, this could confuse users. Consider adding validation for multiple decimal points.♻️ Optional improvement for stricter number validation
if (/\d/.test(char) || (char === "." && i + 1 < chars.length && /\d/.test(chars[i + 1]))) { let num = ""; + let hasDecimal = false; while (i < chars.length && (/\d/.test(chars[i]) || chars[i] === ".")) { + if (chars[i] === ".") { + if (hasDecimal) throw new Error("Invalid number format: multiple decimal points"); + hasDecimal = true; + } num += chars[i++]; } tokens.push({ type: "number", value: parseFloat(num) }); continue; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/next-js-chatbot-starter-template/lib/tools/calculator.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Maintain type safety in TypeScript-first codebase
Never use JSON.stringify; use thesafeStringifyfunction instead, imported from@voltagent/internal
Files:
examples/next-js-chatbot-starter-template/lib/tools/calculator.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (5)
examples/next-js-chatbot-starter-template/lib/tools/calculator.ts (5)
4-8: LGTM!The type definitions are clear and appropriate for the internal tokenizer/parser implementation.
10-47: LGTM - Secure tokenizer with whitelist approach.The tokenizer correctly implements a whitelist approach, only allowing valid mathematical characters. This effectively prevents code injection by rejecting any unexpected characters.
One minor observation: numbers like
1.2.3will be parsed as1.2due toparseFloatbehavior (silently ignoring invalid trailing content), which could be confusing for users but isn't a security concern.
49-118: LGTM - Well-structured recursive-descent parser.The parser correctly implements:
- Standard operator precedence (+/- < */÷ < ** < unary)
- Right-associativity for exponentiation (
2**3**4=2**(3**4))- Proper parentheses handling with error detection for mismatched parens
- Unary operator support
Division by zero returns
Infinity/-Infinityper JavaScript/IEEE 754 semantics, which is acceptable behavior.
120-124: LGTM!Clean orchestration of the tokenize-parse pipeline with proper empty expression handling.
135-150: LGTM - Secure integration of the safe evaluator.The
executefunction now correctly usessafeEvaluateinstead ofeval, eliminating the code injection vulnerability. Error handling is improved by propagating specific error messages to help users understand what went wrong.
Summary
Fix critical code injection vulnerability in
calculator.tsby replacing unsafeeval()with a secure recursive descent math parser.🔴 Vulnerability (Critical)
File:
examples/next-js-chatbot-starter-template/lib/tools/calculator.tsUsing
eval()with user-provided expressions allows arbitrary JavaScript code execution. An attacker could execute malicious code through the calculator tool.Example Attack Vector
✅ Fix
Replaced
eval()with a complete secure math expression parser:tokenize()- Lexical analysis to extract numbers, operators, and parenthesesparseExpression()- Recursive descent parser with proper operator precedenceBefore
After
Supported Operations
2 + 3510 - 463 * 41220 / 542 ** 101024(2 + 3) * 4202 + 3 * 414Testing
References
Summary by cubic
Replaced unsafe eval() in the calculator tool with a safe math expression parser to prevent code injection. Supports +, -, *, /, **, and parentheses with correct precedence.
Written for commit 3f20266. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.